-
Notifications
You must be signed in to change notification settings - Fork 8.2k
scripts: build: Fix check_init_priorities.py for ARCMWDT compatibility #62120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@evgeniy-paltsev , could you also take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first question is why ARCMWDT linker is behaving differently, can you please explain?
MWDT Toolchain is not GCC-based toolchain. Even is has good compatibility in many aspects, it also has some differences. |
|
@gmarull do you have some other questions about this PR? If no, could you please approve? Thanks! |
|
Hi @gmarull, Could you please revisit this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be much safer/readable to move to regexes instead.
import re
...
# before checking each level
expr = re.compile(rf".*{level}(\d+)_(\d+).*")
...
# check if match
m = expr.match(name)
if not m:
continue
priority, sub_priority = m.groups()There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kokas-a I have a PR in flight to rework this logic to not use the object files at all, so it would not rely on the format of the object sections anymore and just needs the boundary symbols in the final elf file, which should be a more robust solution (turns out the current one also breaks with GCC with LTO enabled). It deletes this code entirely, working on some kinks around armclang support but as long as the symbols are there it should work with this as well, could you give it a try? #62459
I'm also open to the idea of only running this for GCC and clang, there's only that much we can do about chasing quirks with non generally available compilers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fabiobaltieri , I have finally try your changes with WMDT compiler. They work fine!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
brilliant, thanks a log, I'll nudge that one to get it unblocked then
zephyrproject-rtos#58388 breaks ARCMWDT building. ARCMWDT linker generates extended section names (ex.: .rela.z_init_POST_KERNEL40_0_.__init_k_sys_work_q_init). Default script "build/check_init_priorities.py" is unable to parse such format. This fixup adds limitation for reading only 2 parameters from priority string. Signed-off-by: Nikolay Agishev <[email protected]>
dbec70d
15fec74 to
dbec70d
Compare
| # ARCMWDT linker generates extended section names | ||
| # (ex.: .rela.z_init_POST_KERNEL40_0_.__init_k_sys_work_q_init). | ||
| # limitation is required for reading only 2 parameters from | ||
| # priority string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment is now redundant, also please check @fabiobaltieri PR, because these changes may not be needed at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fabiobaltieri, @gmarull , Suggested changes #62459 works fine for WMDT compiler. So if pointed PR is applied in nearest future there is no need in this one (#62459).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's merged, hopefully we should be good to go
Sorry about the breakage again, thanks for you patience.
#58388 breaks ARCMWDT building.
ARCMWDT linker generates extended section names
(ex.: .rela.z_init_POST_KERNEL40_0_.__init_k_sys_work_q_init).
Default script "build/check_init_priorities.py" is unable to parse such format. This fixup adds limitation for reading only 2 parameters from priority string.